Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for deprecated attribute and update the presets search dialog #123

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Sarabjeet108
Copy link

@tsmock

I discussed this with Simon about having a filter for the added attributes. We can also have this for region attributes, although I think it is unnecessary in the case of deprecated.

I am having the same issue as with the regions attribute, the validate method in TaggingPresetValidation class is not adding the errors from the check method in TagChecker class.

*/
public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags) {
public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags, boolean displayDeprecated) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, I think we should start using an enum instead of a bunch of booleans.

Example:

Suggested change
public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags, boolean displayDeprecated) {
public enum DisplayCheckboxes {
ONLY_APPLICABLE,
SEARCH_IN_TAGS,
DEPRECATED
}
public TaggingPresetSelector(boolean displayOnlyApplicable, boolean displaySearchInTags) {
this(displayOnlyApplicable ? ONLY_APPLICABLE : null, displaySearchInTags ? SEARCH_IN_TAGS : null);
}
public TaggingPresetSelector(DisplayCheckboxes... options) {
// Many if statements
}

I'll note that my code isn't properly organized, so the enum would go better elsewhere, and I didn't add the class reference in the constructor, but that should (largely) work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a checkbox for regions as well?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, yes. Realistically, the "primary" caller will probably just use DisplayCheckboxes.values() instead of hardcoding the current values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realistically, the "primary" caller will probably just use DisplayCheckboxes.values() instead of hardcoding the current values.

Does that mean we would use the preference for the checkbox associated with the filter like ONLY_APPLICABLE.getPref() instead of using bool value? (getPref will return boolean value)

Comment on lines 234 to 242
if (displayDeprecated) {
ckDeprecated = new JCheckBox();
ckDeprecated.setText(tr("Show deprecated tags"));
pnChecks.add(ckDeprecated);
ckDeprecated.addItemListener(e -> filterItems());
} else {
ckDeprecated = null;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going further on the enums, there are EnumMaps that we should start using, and the translated texts can be done via an enum field (e.g. DEPRECATED(marktr("Show deprecated tags")) + ck.setText(tr(DEPRECATED.getText())).

@@ -254,11 +266,12 @@ protected synchronized void filterItems() {
String text = edSearchText.getText().toLowerCase(Locale.ENGLISH);
boolean onlyApplicable = ckOnlyApplicable != null && ckOnlyApplicable.isSelected();
boolean inTags = ckSearchInTags != null && ckSearchInTags.isSelected();
boolean isDeprecated = ckDeprecated != null && ckDeprecated.isSelected();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving on with the EnumMap idea, this could be done via that. For that matter, the checkbox could have a change listener that puts a boolean in an enum map.

@@ -279,7 +292,7 @@ public static class PresetClassifications implements Iterable<PresetClassificati

private final List<PresetClassification> classifications = new ArrayList<>();

public List<PresetClassification> getMatchingPresets(String searchText, boolean onlyApplicable, boolean inTags,
public List<PresetClassification> getMatchingPresets(String searchText, boolean onlyApplicable, boolean inTags, boolean isDeprecated,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use an enum here. We could probably reuse the one I mentioned earlier (so getMatchingPresets(String searchText, DisplayCheckboxes..., so DisplayCheckboxes is probably the wrong name; see if you can find one yourself).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about SearchStringFilters?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would probably work. But we have other locations where we have search strings. It might be better to have some more identifying information. Although this really depends upon where we put the enum -- if it is in its own file, it will need a much better identifier, such as PresetSearchFilter. If it remains in the TaggingPresetSelector class, then that would be part of the identifier (TaggingPresetSelector.SearchStringFilters.DEPRECATED).

public List<PresetClassification> getMatchingPresets(String[] groupWords, String[] nameWords, boolean onlyApplicable,
boolean inTags, Set<TaggingPresetType> presetTypes, final Collection<? extends OsmPrimitive> selectedPrimitives) {
public List<PresetClassification> getMatchingPresets(String[] groupWords, String[] nameWords, boolean onlyApplicable, boolean inTags,
boolean isDeprecated, Set<TaggingPresetType> presetTypes, final Collection<? extends OsmPrimitive> selectedPrimitives) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably mention (at this point) why I think we should start moving these booleans to an enum: easier readability. There are some other advantages, but function(bool, bool, bool, bool, bool, bool) start to get really hard to wrap your mind around. Some IDEs will give hints as to what each bool is supposed to be, but not all IDEs will.

As a specific example, GitHub doesn't show what a boolean is supposed to be for, so the reviewer/editor has to know the function definition, or go find it.

@Sarabjeet108
Copy link
Author

I am facing a few problems

  1. How will we initialize the values for the filters in the enum map? As a temporary fix, I assigned true to all values using a static block.

  2. You mentioned that the primary caller would call using the .values() method. How will that work if we need the boolean values associated with the filters since .values() will return an array of the enum constants?

@@ -231,7 +232,7 @@ public boolean isValid() {
* selected preset by the user. Every query is of the form ' group/sub-group/.../presetName'
* if the corresponding group of the preset exists, otherwise it is simply ' presetName'.
*/
selector = new TaggingPresetSelector(false, false, false);
selector = new TaggingPresetSelector(PresetSearchFilter.values());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the equivalent of new TaggingPresetSelector(true, true, true), which I don't think you wanted. You probably wanted to just do new TaggingPresetSelector().

Comment on lines 17 to 47
/**
* Map containing the preferences for the filters
*/
private Map<PresetSearchFilter, Boolean> filtersPreference;

static {
for (PresetSearchFilter filter : values()) {
filter.filtersPreference = new EnumMap<>(PresetSearchFilter.class);
filter.filtersPreference.put(filter, true);
}
}

/**
* Sets the preference for the filter
* @param filter the filter to set the preference for
* @param pref true if the filter is enabled, false otherwise
* @since xxx
*/
public void setPref(PresetSearchFilter filter, Boolean pref) {
filtersPreference.put(filter, pref);
}

/**
* Gets the preference for the filter
* @param filter the filter to get the preference for
* @return true if the filter is enabled, false otherwise
* @since xxx
*/
public Boolean getPref(PresetSearchFilter filter) {
return filtersPreference.get(filter);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I said, "we should use an EnumMap", this is not what I was thinking. I'll go find one of the methods using this enum and see if I can hack together something there as an example.

Generally speaking, enum classes should not have a state that can change. There are exceptions, but they are exceptions.

Comment on lines 66 to 68
PresetSearchFilter ONLY_APPLICABLE = PresetSearchFilter.ONLY_APPLICABLE;
PresetSearchFilter SEARCH_IN_TAGS = PresetSearchFilter.SEARCH_IN_TAGS;
PresetSearchFilter DEPRECATED_TAGS = PresetSearchFilter.DEPRECATED_TAGS;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need/want to do this. Importing them statically would work, or you can just use the qualified name (PresetSearchFilter.DEPREACATED_TAGS). I only saw these used in a switch statement, where they aren't needed.

switch(option) {
case ONLY_APPLICABLE: {
ckOnlyApplicable = new JCheckBox();
ckOnlyApplicable.setText(tr(PresetSearchFilter.ONLY_APPLICABLE.getText()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use option.getText() here instead of calling the qualified name.

We want to have a map of option -> checkboxes, so EnumMap<PresetSearchFilter, JCheckBox> would work. Example:

final Map<PresetSearchFilter, JCheckBox> checkboxes = new EnumMap<PresetSearchFilter, JCheckBox>(PresetSearchFilter.class);
for (PresetSearchFilterOption option : options) {
    final JCheckBox box = new JCheckBox();
    box.setText(tr(option));
    box.addItemListener(e -> filterItems());
    pnChecks.add(box);
    switch (option) {
    case SEARCH_IN_TAGS:
        box.setSelected(PREF_SEARCH_IN_TAGS.get()); 
        break;
        // Do other enum values
    }
}

* @param translatedText The translated text associated with the enum constant.
*/
PresetSearchFilter(String translatedText) {
this.translatedText = translatedText;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid confusion, this should probably be textToBeTranslated or something; translatedText implies it has already been translated.

@@ -67,7 +67,7 @@ public static synchronized TaggingPresetSearchPrimitiveDialog getInstance() {
super(MainApplication.getMainFrame(), tr("Search for objects by preset"), tr("Search"), tr("Cancel"));
setButtonIcons("dialogs/search", "cancel");
configureContextsensitiveHelp("/Action/TaggingPresetSearchPrimitive", true /* show help button */);
selector = new TaggingPresetSelector(false, false, false);
selector = new TaggingPresetSelector(PresetSearchFilter.values());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the equivalent of new TaggingPresetSelector(true, true, true), which I don't think you wanted. You probably wanted to just do new TaggingPresetSelector().

Comment on lines 36 to 46
filtersPreference.put(filter, pref);
}

/**
* Gets the preference for the filter
* @param filter the filter to get the preference for
* @return true if the filter is enabled, false otherwise
* @since xxx
*/
public Boolean getPref(PresetSearchFilter filter) {
return filtersPreference.get(filter);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done via the standard Config interface or one of the Property classes.

Comment on lines 274 to 276
final List<PresetClassification> result = classifications.getMatchingPresets(
text, onlyApplicable, inTags, isDeprecated, getTypesInSelection(), selected);
text, ONLY_APPLICABLE.getPref(PresetSearchFilter.ONLY_APPLICABLE), SEARCH_IN_TAGS.getPref(PresetSearchFilter.SEARCH_IN_TAGS),
DEPRECATED_TAGS.getPref(PresetSearchFilter.DEPRECATED_TAGS), getTypesInSelection(), selected);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, you would just use the same EnumMap from one of my other comments (EnumMap<PresetSearchFilter, JCheckBox>) instead.

Example: map.entrySet().stream().filter(entry -> entry.getValue().isSelected().map(Map.Entry::getKey).toArray(PresetSearchFilter[]::new).

@Sarabjeet108 Sarabjeet108 changed the title Update search dialog Add support for deprecated attribute and update the presets search dialog Aug 27, 2023
@@ -121,7 +121,7 @@
<complexType name="item">
<annotation>
<documentation>
Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, icon and name_template are optional attributes.
Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, deprecated, icon, name_template are optional attributes.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, deprecated, icon, name_template are optional attributes.
Every item is one annotation set to select from. name is required, type and preset_name_label are recommended, deprecated, icon, and name_template are optional attributes.


/**
* Set if the preset is deprecated
* @param deprecated if true the preset is deprecated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param deprecated if true the preset is deprecated
* @param deprecated if true the preset is deprecated
* @since xxx

Comment on lines +68 to +74
Map<PresetSearchFilter, Boolean> ckpreferences = new EnumMap<>(PresetSearchFilter.class);
{
//initial value of the checkboxes
ckpreferences.put(PresetSearchFilter.ONLY_APPLICABLE, true);
ckpreferences.put(PresetSearchFilter.SEARCH_IN_TAGS, true);
ckpreferences.put(PresetSearchFilter.DEPRECATED_TAGS, false);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably keep the properties.

Realistically, the map can be populated with ckpreferences.computeIfAbsent(key, k -> PROPERTY.get()).

Using (and saving) the preferences lets us keep the checkbox state between application restarts.

Comment on lines +449 to +456
if (checkboxes.get(PresetSearchFilter.SEARCH_IN_TAGS) != null) {
ckpreferences.put(PresetSearchFilter.SEARCH_IN_TAGS, checkboxes.get(PresetSearchFilter.SEARCH_IN_TAGS).isSelected());
}
if (checkboxes.get(PresetSearchFilter.ONLY_APPLICABLE) != null && checkboxes.get(PresetSearchFilter.ONLY_APPLICABLE).isEnabled()) {
ckpreferences.put(PresetSearchFilter.ONLY_APPLICABLE, checkboxes.get(PresetSearchFilter.ONLY_APPLICABLE).isSelected());
}
if (ckOnlyApplicable != null && ckOnlyApplicable.isEnabled()) {
ONLY_APPLICABLE.put(ckOnlyApplicable.isSelected());
if (checkboxes.get(PresetSearchFilter.DEPRECATED_TAGS) != null && checkboxes.get(PresetSearchFilter.DEPRECATED_TAGS).isEnabled()) {
ckpreferences.put(PresetSearchFilter.DEPRECATED_TAGS, checkboxes.get(PresetSearchFilter.DEPRECATED_TAGS).isSelected());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to keep the PREFERENCE.put method calls.


/**
* This enum defines different filters for searching presets.
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*/
* @since xxx
*/

Comment on lines +37 to +38
selector = new TaggingPresetSelector(PresetSearchFilter.ONLY_APPLICABLE, PresetSearchFilter.SEARCH_IN_TAGS,
PresetSearchFilter.DEPRECATED_TAGS);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
selector = new TaggingPresetSelector(PresetSearchFilter.ONLY_APPLICABLE, PresetSearchFilter.SEARCH_IN_TAGS,
PresetSearchFilter.DEPRECATED_TAGS);
selector = new TaggingPresetSelector(PresetSearchFilter.values());


/**
* Called from the {@code TaggingPresetConstructor}.
*
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
*
* @since xxx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants